Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(compiler)!: Remove Exclusive/Inclusive Ranges, provide as record via compiler #1616

Merged
merged 9 commits into from
Feb 22, 2023

Conversation

phated
Copy link
Member

@phated phated commented Jan 22, 2023

Closes #1404

This adds the Range enum as a language-supplied type, which is a prerequisite to having language syntax for ranges.

The new Range is a record Range<a> { rangeStart: a, rangeEnd: a }. It is now up to APIs to decide if the range is interpreted as Inclusive or Exclusive, and is no longer limited to only Numbers, which also allow APIs to decide how to interpret the types.

@alex-snezhko
Copy link
Member

I think if Exclusive ranges were to be kept then special syntax should be added for them, otherwise I think it'd be better to remove them altogether as it's awkward to have syntax for one way but not the other (also doing end - 1 for exclusive ranges isn't too big of a deal at the end of the day).

Also, thoughts on adding unbounded ranges like 3..? I find them pretty useful in other languages that have them for slicing and whatnot (this would also allow the various slice functions to take a Range rather than a second optional argument as discussed in #1566 which seems nicer imo). Range.map could then be replaced with a generator function instead of returning a list to accommodate for not being able to create an infinite list (or an exception could just be thrown in this case 🤷 )

@phated
Copy link
Member Author

phated commented Feb 22, 2023

I've updated this PR to turn Range into record Range<a> { range_start: a, range_end: a } as we discussed on the community call. It is now up to APIs to decide if the range is interpreted as Inclusive or Exclusive, and is no longer limited to only Numbers, which also allow APIs to decide how to interpret the types.

@phated phated changed the title feat(compiler)!: Include Range as language-supplied type feat(compiler)!: Remove Exclusive/Inclusive Ranges, provide as record via compiler Feb 22, 2023
@alex-snezhko
Copy link
Member

Should the casing of the record fields be changed to camelcase to match the rest of the language?

@phated
Copy link
Member Author

phated commented Feb 22, 2023

Probably, but I hate it. 🤣

@alex-snezhko
Copy link
Member

What about just start and end? Since Range will become a builtin type then maybe the range prefix can just be omitted by the same logic that Some is not called OptionSome, Ok is not called ResultOk, etc?

@phated
Copy link
Member Author

phated commented Feb 22, 2023

We discussed start and end but it's not a good idea to use commonly used words in records provided by the compiler because they will result in ambiguous record warnings when people write custom records with those common words.

@ospencer
Copy link
Member

Thoughts on Inclusive and Exclusive submodules (or just an inclusive submodule)? I feel like it'd be pretty nice to be able to do Range.Inclusive.inRange() in those times I want an inclusive range.

@phated
Copy link
Member Author

phated commented Feb 22, 2023

just an inclusive submodule

I like the idea. I suppose I need to go back and get that implementation 😛

Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome!

compiler/src/typed/builtin_types.re Outdated Show resolved Hide resolved
* @since v0.6.0
* @history v0.3.0: Root APIs originally handled Inclusive & Exclusive variants
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the docs touch the implementations or is this a formatter bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatter bug. We're not blocking on it though. Hopefully it'll be fixed soon.

Co-authored-by: Oscar Spencer <oscar@grain-lang.org>
Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@phated phated added this pull request to the merge queue Feb 22, 2023
Merged via the queue into main with commit 49a399d Feb 22, 2023
@phated phated deleted the phated/compiler-range-type branch February 22, 2023 17:21
av8ta pushed a commit to av8ta/grain that referenced this pull request Apr 11, 2023
… via compiler (grain-lang#1616)

* feat(compiler)!: Remove Exclusive/Inclusive Ranges, provide as record via compiler

* update snapshots

* update docs

* formatting

* rename range_start and range_end

* remove old idents

* feat(stdlib): Add Inclusive submodule to Range module

* Update compiler/src/typed/builtin_types.re

Co-authored-by: Oscar Spencer <oscar@grain-lang.org>

* formatting

---------

Co-authored-by: Oscar Spencer <oscar@grain-lang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Range enum should be provided by compiler
3 participants